Skip to content

Conversation

amirreza8002
Copy link
Member

@amirreza8002 amirreza8002 commented Nov 14, 2024

this is the initial version, there are more things that need to be done
i'll update the docs and README after i'm sure the code works

i've tried to make valkey an optional dependency,
loosen up redis so other servers can be configured easier
and changed some names to be more abstract

waiting for some feedback 👐🏼

related issue: #189

@amirreza8002 amirreza8002 force-pushed the val branch 3 times, most recently from bef78ae to 462d942 Compare November 14, 2024 15:20
@amirreza8002 amirreza8002 marked this pull request as ready for review November 14, 2024 15:44
Copy link
Member

@cunla cunla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, overall looks good and a welcomed change.

I added a few comments regarding where things should be defined.
The one thing I am not very clear on is why is libvalkey needed?

@amirreza8002
Copy link
Member Author

I'll take a look at the comments

libvalkey is not needed per say

it's an optional C-binding that improves performance of valkey-py
thought it's good to include, but we can remove it if you want

@cunla
Copy link
Member

cunla commented Nov 16, 2024

Hi, I made some changes - let me know what you think.

@cunla cunla merged commit 5e6428c into django-commons:master Nov 16, 2024
26 checks passed
@amirreza8002
Copy link
Member Author

LGTM
I'll report if i see anything new then✌️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants